feat(notebook-migration, k8s): deploy the notebook migration tool on Kubernetes - #8073
Draft
zyratlo wants to merge 29 commits into
Draft
feat(notebook-migration, k8s): deploy the notebook migration tool on Kubernetes#8073zyratlo wants to merge 29 commits into
zyratlo wants to merge 29 commits into
Conversation
# Conflicts: # sql/changelog.xml
…migration-tool-kubernetes
github-actions
Bot
added
dependencies
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 526 | 0.321 | 18,760/22,786/22,786 us | 🔴 +19.7% / 🔴 +68.0% |
| 🔴 | bs=100 sw=10 sl=64 | 1,136 | 0.693 | 84,562/112,320/112,320 us | 🔴 +17.6% / 🔴 +18.2% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,342 | 0.819 | 744,037/788,116/788,116 us | ⚪ within ±5% / 🟢 -16.8% |
Baseline details
Latest main bdc6d2a from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 526 tuples/sec | 568 tuples/sec | 894.27 tuples/sec | -7.4% | -41.2% |
| bs=10 sw=10 sl=64 | MB/s | 0.321 MB/s | 0.347 MB/s | 0.546 MB/s | -7.5% | -41.2% |
| bs=10 sw=10 sl=64 | p50 | 18,760 us | 15,667 us | 11,164 us | +19.7% | +68.0% |
| bs=10 sw=10 sl=64 | p95 | 22,786 us | 28,065 us | 14,219 us | -18.8% | +60.2% |
| bs=10 sw=10 sl=64 | p99 | 22,786 us | 28,065 us | 16,863 us | -18.8% | +35.1% |
| bs=100 sw=10 sl=64 | throughput | 1,136 tuples/sec | 1,209 tuples/sec | 1,158 tuples/sec | -6.0% | -1.9% |
| bs=100 sw=10 sl=64 | MB/s | 0.693 MB/s | 0.738 MB/s | 0.707 MB/s | -6.1% | -2.0% |
| bs=100 sw=10 sl=64 | p50 | 84,562 us | 81,586 us | 88,711 us | +3.6% | -4.7% |
| bs=100 sw=10 sl=64 | p95 | 112,320 us | 95,531 us | 95,062 us | +17.6% | +18.2% |
| bs=100 sw=10 sl=64 | p99 | 112,320 us | 95,531 us | 102,275 us | +17.6% | +9.8% |
| bs=1000 sw=10 sl=64 | throughput | 1,342 tuples/sec | 1,362 tuples/sec | 1,188 tuples/sec | -1.5% | +13.0% |
| bs=1000 sw=10 sl=64 | MB/s | 0.819 MB/s | 0.831 MB/s | 0.725 MB/s | -1.4% | +13.0% |
| bs=1000 sw=10 sl=64 | p50 | 744,037 us | 733,377 us | 874,204 us | +1.5% | -14.9% |
| bs=1000 sw=10 sl=64 | p95 | 788,116 us | 773,315 us | 916,845 us | +1.9% | -14.0% |
| bs=1000 sw=10 sl=64 | p99 | 788,116 us | 773,315 us | 947,186 us | +1.9% | -16.8% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,380.15,200,128000,526,0.321,18759.97,22786.01,22786.01
1,100,10,64,20,1760.75,2000,1280000,1136,0.693,84562.42,112320.47,112320.47
2,1000,10,64,20,14908.60,20000,12800000,1342,0.819,744036.68,788115.67,788115.67
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8073 +/- ##
============================================
- Coverage 93.51% 93.50% -0.01%
- Complexity 4709 4747 +38
============================================
Files 1181 1187 +6
Lines 47809 47935 +126
Branches 5327 5343 +16
============================================
+ Hits 44708 44824 +116
+ Misses 1663 1662 -1
- Partials 1438 1449 +11
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
The notebook migration tool runs in the single-node Docker stack (#7931) and in local development (#7933), but the Helm chart has no
notebook-migration-serviceand no JupyterLab. A Kubernetes deployment shows the tool in the workspace with nothing serving its endpoints. This adds both, following the chart's existing convention that orchestrator services are global and stateful resources are per user.notebook-migration-servicebecomes a global Deployment alongside the other services, with a ServiceAccount whose Role is scoped to the JupyterLab namespace only. Per-user JupyterLab pods live in their own namespace behind a headless Service, addressed asjupyter-<uid>.<service>.<namespace>.svc.cluster.local, with a ResourceQuota bounding the pool and an optional prepull DaemonSet. A NetworkPolicy stops one user's pod reaching another's. Onevalues.yamlswitch,notebookMigrationService.enabled, gates the Deployment, the routes, the pool, and the button in the workspace, and it defaults to off.How browser traffic reaches the right pod
This is the design decision worth reviewing closely. JupyterLab is loaded in an iframe and then issues its own requests for assets, contents and kernel websockets. None of those can carry a Texera token, and Texera has no session cookie, so the caller cannot be authenticated per request. Each user's JupyterLab is therefore served under
/jupyter/<uid>/, and the gateway's existing ExtAuthz hook resolves that uid to the pod's recorded address and rewritesHost. It reusesdynamic-routes, itsDynamicResolverbackend, and theSecurityPolicythat already forwardsHost, so no new gateway machinery is introduced.That mechanism routes; it does not authorize. What keeps users apart is the per-user JupyterLab token from #7665, derived from a server held secret and unguessable. Anyone who can reach the gateway can route to any user's pod and will get a 403 from JupyterLab without that user's token. The NetworkPolicy is defence in depth on top, closing the one case where a genuinely hostile neighbour runs: another user's pod. This is a weaker posture than a per-request authorization check, and it is stated plainly in the code rather than left to be inferred.
This PR is not Helm only, unlike what #8006 anticipated. Four backend changes are inseparable from a working deployment:
/apiincluded.access-control-servicegains the Jupyter case inauthorize().Hostmakes JupyterLab's own cross origin check fail on cookie authenticated requests, which blocked kernel startup entirely.One change reaches beyond the tool: the gateway route serving
/api/chatand/api/modelshad no explicit timeout, so Envoy's 15 second default applied. LLM completions routinely run past that, and the upstream call succeeds and is then discarded, so every conversion failed while burning the API call. The route now takes its timeout fromgatewayConfig.llmRequestTimeout, defaulting to 10m to matchpython-notebook-migration-timeout-minutes. This also fixes the agent chat, which shares the route.Known limitations, both worth their own issues:
work/. Notebooks survive in the database but are not re-uploaded automatically. The same is true of a single-node container restart, so this is not specific to Kubernetes.Any related issues, documentation, discussions?
Closes #8006
Parent issue #4301
Depends on #7665 (PR #8032), which is still open: without per-user resolution a global service would hand every user the same JupyterLab and token, which is exactly what this topology exists to prevent. This branch is stacked on that work, so the diff will shrink once #8032 merges. Roadmap context is the architectural note on #5258.
How was this PR tested?
Unit tests pass across the touched modules:
Config/test(71),NotebookMigrationService/test(107),AccessControlService/test(62), withscalafmtCheckAlland a repo wideTest/compileclean.helm templaterenders in both modes, and with the feature disabled the chart emits no Jupyter or notebook-migration objects at all.Deployed and exercised end to end on a local Minikube cluster running Calico, chosen because Minikube's default CNI creates NetworkPolicy objects without enforcing them, which would make the isolation commit look correct while enforcing nothing. Images were built from this branch, since the published
latestimages predate #8032.Verified on that cluster: the feature flag reaches the frontend; opening the panel provisions
jupyter-<uid>and moves the quota; the issued token matches an independent HMAC derivation of the same uid; the pod's labels, hostname and subdomain match what the headless Service selects;/jupyter/<uid>/reaches the right pod through the gateway while the same URL without a token is redirected to JupyterLab's login; the iframe renders and cell click sync works in both directions.Isolation was tested with an ablation rather than a single observation. With the policy in place,
jupyter-1cannot reachjupyter-2on 8888 by pod IP or by DNS, in both directions, while the same pod reaches another namespace in the same probe, which rules out a broken pod or DNS. Removing the policy makes the connection succeed and restoring it blocks again.The stale row path from #7665 was also confirmed here: deleting a pod without touching its registry row causes the next request to log the address as unreachable, discard the row and provision a replacement.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)